Fix TestCase::expectErrorLog() with open_basedir php.ini - #6208
Conversation
| * - https://github.com/php/php-src/issues/17817 | ||
| * - https://github.com/php/php-src/issues/18530 | ||
| * | ||
| * Until then, ignore TestCase::expectErrorLog() if open_basedir php.ini is in effect. |
There was a problem hiding this comment.
ignoring expectations sounds like a bad idea.. maybe we should turn such a test "risky" and/or can introduce a warning telling the author of the test, that not everything worked according to plan.
siltent ignore sounds like a bad idea.
I guess @sebastianbergmann has an opinion on that one
There was a problem hiding this comment.
Good point.
"incomplete" should be the right status. If error_log file could not be created, test would still complete but will be marked as incomplete.
There was a problem hiding this comment.
Should the "throw incomplete exception" be moved then into TestCase::runBare to always run https://github.com/sebastianbergmann/phpunit/blob/12.1.5/src/Framework/TestCase.php#L500-L502?
Another option is to revert/remove the newly introduced TestCase::expectErrorLog() until php-src allows to check it without temporary file. Currently, it suffers not only the issue here, but it is also not the fastest to having to create temporary file before each TestCase run.
There was a problem hiding this comment.
but it is also not the fastest to having to create temporary file before each TestCase run.
please share a benchmark which proves your claim - noone else reported something like that since the change was released. how big is the impact?
There was a problem hiding this comment.
<?php
$t1 = 0;
$t2 = 0;
for ($i = 0; $i < 1000; ++$i) {
$ts = microtime(true);
$capture = tmpfile();
$ts2 = microtime(true);
$t1 += $ts2 - $ts;
$path = stream_get_meta_data($capture)['uri'];
$t2 += microtime(true) - $ts2;
}
var_dump($t1 / $i);
var_dump($t2 / $i);
T2 does not call any FS, T1 is about 0.4 ms / per iteration (TestCase run).
There was a problem hiding this comment.
I mean a real world impact, not microbenchmarks.
I am aware that not running stuff is faster then running stuff, but thats not helpful.
There was a problem hiding this comment.
In another words about 1 second extra time per 2500 tests :)
This is the real world overhead of requiring FS.
Let @sebastianbergmann decide this thread incl. what to throw.
|
I hope to find the time to look into this within the next week. |
|
@sebastianbergmann it seems you deleted the target brach without retargetting this PR first. Please have a look on this PR, currently we cannot upgrade to PHPUnit 12 everywhere because of this issue. |
|
I am so sorry about that! I need to add branch retargeting to my list of things to consider when releasing a new minor version and will try my best to avoid a situation like this in the future. That being said, I cannot reopen this pull request as the target branch no longer exists. Therefore, I cannot change the target of this pull request to Can you please send a new pull request against |
|
👍 I cannot tell if the target branch will be recreated, if the PR could be reopened then or not. I will create another one into |
fix #6197